Skip to content

stream: add more filter tests #41936

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
wants to merge 3 commits into from
Closed

stream: add more filter tests #41936

wants to merge 3 commits into from

Conversation

benjamingr
Copy link
Member

Add more tests to Readable.prototype.filter

cc @nodejs/streams @aduh95

@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. test Issues and PRs related to the tests. labels Feb 11, 2022
@benjamingr benjamingr mentioned this pull request Feb 12, 2022
Closed
@benjamingr benjamingr added the request-ci Add this label to start a Jenkins CI on a PR. label Feb 12, 2022
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Feb 12, 2022
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/42515/

aduh95
aduh95 reviewed Feb 12, 2022
View reviewed changes
test/parallel/test-stream-filter.js
});
assert.rejects(
stream.filter(() => true).toArray(),
/boom/,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we would want to check the error by reference here, to match the spec proposal text.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I recall this discussion before but I don't recall the conclusion, do you happen to have a reference?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#41642 (comment)

mcollina
mcollina approved these changes Feb 12, 2022
View reviewed changes
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@github-actions github-actions bot mentioned this pull request Feb 13, 2022
Open
19 tasks
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/42520/

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/42525/

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/42529/

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/42533/

@aduh95
Copy link
Contributor

aduh95 commented Feb 13, 2022

There's a CI failure related to the changes in this PR:

AssertionError [ERR_ASSERTION]: Expected values to be strictly equal:

4 !== 2

    at Immediate._onImmediate (/home/iojs/build/workspace/node-test-binary-arm/test/parallel/test-stream-filter.js:136:12)
    at processImmediate (node:internal/timers:466:21) {
  generatedMessage: true,
  code: 'ERR_ASSERTION',
  actual: 4,
  expected: 2,
  operator: 'strictEqual'
}

@github-actions github-actions bot mentioned this pull request Feb 14, 2022
Open
18 tasks
@benjamingr
Copy link
Member Author

@aduh95 that's actually not related to this PR - but I can see why that'd happen I'll push a fix (which again is unrelated to this PR).

@benjamingr benjamingr added the request-ci Add this label to start a Jenkins CI on a PR. label Feb 14, 2022
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Feb 14, 2022
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/42544/

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/42548/

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/42554/

@benjamingr benjamingr added the commit-queue Add this label to land a pull request using GitHub Actions. label Feb 14, 2022
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Feb 14, 2022
@nodejs-github-bot
Copy link
Collaborator

Commit Queue failed
- Loading data for nodejs/node/pull/41936
✔  Done loading data for nodejs/node/pull/41936
----------------------------------- PR info ------------------------------------
Title      stream: add more filter tests (#41936)
Author     Benjamin Gruenbaum  (@benjamingr)
Branch     benjamingr:add-filter-tests -> nodejs:master
Labels     test, needs-ci
Commits    3
 - stream: add more filter tests
 - fixup!
 - fixup! remove timeout from test
Committers 1
 - Benjamin Gruenbaum 
PR-URL: https://github.com/nodejs/node/pull/41936
Reviewed-By: Robert Nagy 
Reviewed-By: James M Snell 
Reviewed-By: Matteo Collina 
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/41936
Reviewed-By: Robert Nagy 
Reviewed-By: James M Snell 
Reviewed-By: Matteo Collina 
--------------------------------------------------------------------------------
   ⚠  Commits were pushed since the last review:
   ⚠  - fixup! remove timeout from test
   ℹ  This PR was created on Fri, 11 Feb 2022 17:55:59 GMT
   ✔  Approvals: 3
   ✔  - Robert Nagy (@ronag) (TSC): https://github.com/nodejs/node/pull/41936#pullrequestreview-880871183
   ✔  - James M Snell (@jasnell) (TSC): https://github.com/nodejs/node/pull/41936#pullrequestreview-880872056
   ✔  - Matteo Collina (@mcollina) (TSC): https://github.com/nodejs/node/pull/41936#pullrequestreview-880887559
   ✔  Last GitHub CI successful
   ℹ  Last Full PR CI on 2022-02-14T14:26:06Z: https://ci.nodejs.org/job/node-test-pull-request/42554/
- Querying data for job/node-test-pull-request/42554/
   ✔  Last Jenkins CI successful
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu
https://github.com/nodejs/node/actions/runs/1842083455

@nodejs-github-bot nodejs-github-bot added the commit-queue-failed An error occurred while landing this pull request using GitHub Actions. label Feb 14, 2022
benjamingr added a commit that referenced this pull request Feb 14, 2022
@benjamingr
stream: add more filter tests
da11381 
PR-URL: #41936
Reviewed-By: Robert Nagy <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
@benjamingr
Copy link
Member Author

Forgot to autosquash, meh, landed manually :)

Landed in da11381 🎉

@benjamingr benjamingr closed this Feb 14, 2022
@aduh95
Copy link
Contributor

aduh95 commented Feb 14, 2022

Forgot to autosquash, meh

FYI you can use commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. to ask the CQ to squash all the commits when landing a PR.

@benjamingr
Copy link
Member Author

Thanks, yeah, I forgot to use that and already had a terminal open with node so I just landed with ncu :)

This was referenced Feb 15, 2022
Open
Open
Open
bengl pushed a commit to bengl/node that referenced this pull request Feb 21, 2022
@benjamingr @bengl
stream: add more filter tests
8949daa 
PR-URL: nodejs#41936
Reviewed-By: Robert Nagy <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
bengl pushed a commit to bengl/node that referenced this pull request Feb 21, 2022
@benjamingr @bengl
stream: add more filter tests
db2dc99 
PR-URL: nodejs#41936
Reviewed-By: Robert Nagy <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
@bengl bengl mentioned this pull request Feb 21, 2022
Merged
bengl pushed a commit that referenced this pull request Feb 21, 2022
@benjamingr @bengl
stream: add more filter tests
6c9a36c 
PR-URL: #41936
Reviewed-By: Robert Nagy <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
bengl pushed a commit that referenced this pull request Feb 22, 2022
@benjamingr @bengl
stream: add more filter tests
f5d7339 
PR-URL: #41936
Reviewed-By: Robert Nagy <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
@danielleadams
Copy link
Contributor

@benjamingr this breaks when landing on v16.x-staging. Can you open a backport PR to land on v16.x? Thank you

targos pushed a commit that referenced this pull request Jul 31, 2022
@benjamingr @targos
stream: add more filter tests
c4dc8b6 
PR-URL: #41936
Reviewed-By: Robert Nagy <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
@targos targos mentioned this pull request Aug 3, 2022
Merged
guangwong pushed a commit to noslate-project/node that referenced this pull request Oct 10, 2022
@benjamingr @guangwong
stream: add more filter tests
1ea8af3 
PR-URL: nodejs/node#41936
Reviewed-By: Robert Nagy <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aduh95 aduh95 aduh95 left review comments

@mcollina mcollina mcollina approved these changes

@jasnell jasnell jasnell approved these changes

@ronag ronag ronag approved these changes

Assignees
No one assigned
Labels
commit-queue-failed An error occurred while landing this pull request using GitHub Actions. needs-ci PRs that need a full CI run. test Issues and PRs related to the tests.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@benjamingr @nodejs-github-bot @aduh95 @danielleadams @mcollina @jasnell @ronag @targos